-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SysCaptureBinary: decode in writeorg #6880
Conversation
Upstream: pytest-dev#6880 Ref (fixes): pytest-dev#6871 (cherry picked from commit d90ae38)
Upstream: pytest-dev#6880 Ref (fixes): pytest-dev#6871
Upstream: pytest-dev#6880 Ref (fixes): pytest-dev#6871
Upstream: pytest-dev#6880 Ref (fixes): pytest-dev#6871
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the Capture types. My conclusion is that there is a lot of confusion there between str
/bytes
and TextIO
/BinaryIO
. And it's exacerbates by EncodedFile
which is some weird cross between the two.
I think this should be "attacked" as follows:
- Rework
EncodedFile
to be sane - I do this in Remove safe_text_dupfile() and simplify EncodedFile #6899 (though still need to investigate some CI failure). - To fix the immediate bug, integrate a version of this PR. I think it needs to consider all of
snap()
andwriteorg()
for all of{FD,Sys}Capture{,Binary}
. - Type annotate these classes. This will reveal the faulty inheritance design which breaks Liskov Substitution and makes things very confusing (mypy will complain). Not to be committed - see next step.
- Refactor to remove the faulty inhertiance and make everything properly typed, and ensuring the tests pass.
@@ -681,7 +681,8 @@ def resume(self): | |||
setattr(sys, self.name, self.tmpfile) | |||
self._state = "resumed" | |||
|
|||
def writeorg(self, data): | |||
def writeorg(self, data: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be bytes
. It seems like the data = data....
confused mypy so it didn't catch it.
@@ -681,7 +681,8 @@ def resume(self): | |||
setattr(sys, self.name, self.tmpfile) | |||
self._state = "resumed" | |||
|
|||
def writeorg(self, data): | |||
def writeorg(self, data: str) -> None: | |||
data = data.decode(self._old.encoding) | |||
self._old.write(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe write to self._old.buffer
instead of doing the decode
? Although it might not exist in case of "nested" patching of sys.stdout
and friends (e.g. if someone patches sys.stdout
to a io.StringIO
, which doesn't have an underlying buffer, and then tries to capture it -- probably not a proper thing to do).
Fixes #6871.